Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Pipelines] Processors editor a11y focus states #79122

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Oct 1, 2020

Summary

  • Updated structure of HTML (removed unnecessary flex group and items)
  • Optimised keyboard focus states in move mode
  • Updated styling to better cater for overflow in certain cases

Addresses items 2 & 3 of #71666

Test

Pipeline JSON
{
  "processors": [
    {
      "grok": {
        "field": "my-field",
        "patterns": [
          "enter"
        ],
        "description": "test 2"
      }
    },
    {
      "grok": {
        "field": "test",
        "patterns": [
          "test",
          "test-again"
        ],
        "description": "test 1",
        "on_failure": [
          {
            "bytes": {
              "field": "test"
            }
          }
        ]
      }
    }
  ]
}
  1. Navigate to create pipeline processor in Ingest pipelines plugin
  2. Import processors from above
  3. Tab to the move button of the inner "Bytes" processor
  4. Use the keyboard navigation to either cancel the move (can be cancelled with esc key too) or place the processor somewhere else. Using spacebar+tab to interact with the buttons and drop zones.

Release note

Optimised keyboard navigation of the ingest processors component

Gif

a11y-focus-states-ingest-proc-editor

Checklist

Delete any items that are not applicable to this PR.

- fix use of flex items (removed unnecessary use thereof)
- also fixed overflow when tabbing through drop zones (compressed)
@jloleysens jloleysens added release_note:enhancement v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 Feature:Ingest Node Pipelines Ingest node pipelines management labels Oct 1, 2020
@jloleysens jloleysens requested review from a team as code owners October 1, 2020 14:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens jloleysens changed the title [Ingest Pipelines] Processors editor focus states [Ingest Pipelines] Processors editor a11y focus states Oct 2, 2020
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. Thanks for making these improvements @jloleysens!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding and removing tabindex properties from semantically both interact-able (button) and non-interact-able (div) elements seems like a big accessibility issue. Has this been tested with screen-readers?

@jloleysens
Copy link
Contributor Author

Hi @cchaos ! In this issue #71666 @myasonik and I went through this component with a screen reader and identified this as something that we can optimise for navigating with keyboard.

This PR specifically addresses points 2 & 3 of that issue. Turning tabindex off for certain otherwise tabbable components enables faster traversal with a keyboard to the "drop zones".

Does this answer your question or was there a specific concern that I hadn't considered in the proposed changes?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the history and explanation! So long as there's been discussion and thought towards a11y 👍

@myasonik myasonik self-requested a review October 2, 2020 16:21
@myasonik
Copy link
Contributor

myasonik commented Oct 2, 2020

Hey, sorry to hold this up a touch, but I want to give this a pass through. Will try to get to it before end of day.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
ingestPipelines 812.9KB 812.9KB +4.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This doesn't fix all the a11y issues on the page but it does improve the state of things for keyboard users so it's best to merge

For a little more context on the screen reader front, a lot of problems on the page stem from using react-virtualized so the situation won't really be improved there until we can swap to using react-window

@jloleysens jloleysens merged commit f6729dc into elastic:master Oct 5, 2020
@jloleysens jloleysens deleted the ingest-pipelines/processors-editor-fix-focus branch October 5, 2020 15:05
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 5, 2020
* Fix showing of accessibility border

- fix use of flex items (removed unnecessary use thereof)
- also fixed overflow when tabbing through drop zones (compressed)

* refactor isLast to compressed

* optimize keyboard focus states in move mode

* fix jest test

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 5, 2020
…nes/fix-description-field

* 'master' of github.com:elastic/kibana:
  A11y tests for user page (elastic#79199)
  [Ingest Pipelines] Processors editor a11y focus states (elastic#79122)
  [Ingest pipelines] Clean up component integration tests (elastic#78838)
  Drilldowns in examples (elastic#75640)
  Storybook and Jest cleanup (elastic#79305)
  adds EQL sequence rule test (elastic#79287)
  PR template a11y checklist item improvement (elastic#79243)
  [Security Solution] Adding tests for dns pipeline in the endpoint package (elastic#79177)
  [ML] Only adjust the bounds of SMV if annotations are visible (elastic#79210)
  global search to ts refs (elastic#79446)
  [Index management] Update TemplateDeserialized interface (elastic#78913)
  [Telemetry] server fetcher check all collectors ready before sending (elastic#79398)
  [Mappings editor] Fix app crash when selecting "other" field type (elastic#79434)
  [`/api/stats`] Add documentation + small improvement (elastic#79330)
  [Discover] "View surrounding documents" encodes spaces in filters (elastic#79283)
  [Lens] refactor DimensionContainer and fix flyout bug (elastic#79277)

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/pipeline_processors_editor_item/inline_text_input.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/processors_tree/components/private_tree.tsx
jloleysens added a commit that referenced this pull request Oct 5, 2020
* Fix showing of accessibility border

- fix use of flex items (removed unnecessary use thereof)
- also fixed overflow when tabbing through drop zones (compressed)

* refactor isLast to compressed

* optimize keyboard focus states in move mode

* fix jest test

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Ingest Node Pipelines Ingest node pipelines management release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants